Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: deprecating pubsub topic #2997

Merged
merged 33 commits into from
Sep 10, 2024
Merged

chore: deprecating pubsub topic #2997

merged 33 commits into from
Sep 10, 2024

Conversation

gabrielmer
Copy link
Contributor

@gabrielmer gabrielmer commented Aug 26, 2024 •
edited
Loading

Description

Deprecating the pubsub-topic CLI config in favor of cluster-id, shards and a new config item network-shards stating the amount of shards in the given cluster-id for autosharding purposes

The word pubsubTopic in our codebase will refer solely to the string representation of clusterId+shard that we use in the libp2p layer.

Changes

  • marking pubsub topic configuration as deprecated
  • converting pubsub topics to shards for compatibility during the deprecation period
  • using RelayShard type instead of pubsub topic strings in Waku's higher layers
  • Introducing network-shards configuration item for autosharding
  • Using the max shard number + 1 as network-shards if not configured

Issue

closes #2806

@github-actions GitHub Actions
Copy link

This PR may contain changes to configuration options of one of the apps.

If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed.

Please also make sure the label release-notes is added to make sure any changes to the user interface are properly announced in changelog and release notes.

@gabrielmer gabrielmer mentioned this pull request Aug 26, 2024
Closed
@gabrielmer gabrielmer added release-notes Issue/PR needs to be evaluated for inclusion in release notes highlights or upgrade instructions E:nwaku Sharding peer management and discovery labels Aug 26, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 26, 2024 •
edited
Loading

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2997

Built from 281af5b

@gabrielmer gabrielmer force-pushed the chore-deprecating-pubsub-topic branch from 471bb6d to 3f92ea7 Compare August 28, 2024 22:11
@gabrielmer
Copy link
Contributor Author

Didn't change the terminology in the liteprotocoltester to avoid breaking compatibility in current tests. Should we change there too all references to pubsub topic to cluster and shard? @NagyZoltanPeter

@gabrielmer gabrielmer marked this pull request as ready for review August 29, 2024 15:25
@NagyZoltanPeter
Copy link
Contributor

Didn't change the terminology in the liteprotocoltester to avoid breaking compatibility in current tests. Should we change there too all references to pubsub topic to cluster and shard? @NagyZoltanPeter

If that ok I will pull after next week!

SionoiS
SionoiS approved these changes Aug 29, 2024
View reviewed changes
Copy link
Contributor

@SionoiS SionoiS left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Nice work!

Left some suggestions.

waku/factory/external_config.nim Outdated Show resolved Hide resolved
waku/factory/node_factory.nim Outdated Show resolved Hide resolved
Ivansete-status
Ivansete-status approved these changes Aug 29, 2024
View reviewed changes
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for it! 💯
Just added a few minor comments that I hope you find useful

apps/chat2/config_chat2.nim Outdated Show resolved Hide resolved
tests/testlib/wakunode.nim Outdated Show resolved Hide resolved
waku/factory/external_config.nim
Comment on lines +320 to +324
desc:
"Deprecated. Default pubsub topic to subscribe to. Argument may be repeated.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create a release milestone and take note of the final deprecation. Maybe in v0.34.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have #2806 open, just changed the milestone to v0.34.0

Maybe we shouldn't close this issue after this PR? or create a new one? Any of them works for me

waku/factory/node_factory.nim Outdated Show resolved Hide resolved
waku/factory/external_config.nim Outdated Show resolved Hide resolved
waku/factory/node_factory.nim Outdated Show resolved Hide resolved
waku/factory/waku.nim Outdated Show resolved Hide resolved
waku/factory/waku.nim Outdated Show resolved Hide resolved
@gabrielmer gabrielmer force-pushed the chore-deprecating-pubsub-topic branch from 086710f to 0e869d5 Compare August 29, 2024 18:03
chaitanyaprem
chaitanyaprem reviewed Sep 3, 2024
View reviewed changes
Copy link
Contributor

@chaitanyaprem chaitanyaprem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, as config was confusing otherwise

waku/factory/node_factory.nim Outdated Show resolved Hide resolved
jm-clius
jm-clius approved these changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally agree with this direction. The concept of pubsub topic remains useful within the wire protocols itself as it matches more closely what is modelled in libp2p.

@gabrielmer
adding default pubsub topic
0db34d2
@gabrielmer
fix syntax error
57f5ef0
@gabrielmer
fix type
be46c5e
@gabrielmer
fix typo
80e1ba1
@gabrielmer
fix more compilation errors
f932b78
@gabrielmer
fix variable name
0fccc84
@gabrielmer
fixing chat2 compilation
1b0ce73
@gabrielmer
fixing tests
9d2134b
@gabrielmer
fix compilation error
3913762
@gabrielmer
more compilation errors
0e91c27
@gabrielmer
more fixes
f42865e
@gabrielmer
fix type
5bb390e
@gabrielmer
fix
dd85361
@gabrielmer
more test fixes
ecb771f
@gabrielmer
fix types
c64ecc5
@gabrielmer
fix name
98e0681
@gabrielmer
fix naming
528a7c9
@gabrielmer
fix networkmonitor compilation
f0ecd1e
@gabrielmer
adding temporary support to pubsub topics
ef22c8c
@gabrielmer
fixing shards validation
f232070
@gabrielmer
removing unnecessary chat2bridge config and improving default node do…
8688ec8 
…cumentation
@gabrielmer
improving logs and using defaults
dbcbf13
@gabrielmer
changing networkShards to numShardsInNetwork
8029406
@gabrielmer
apply suggestions
a86bb90
@gabrielmer
log warning when automagically setting numShardsInNetwork
3f78bfe
@gabrielmer
setting 1024 shards as default if users don't set it
c6e7332
@gabrielmer gabrielmer merged commit a3cd2a1 into master Sep 10, 2024
9 of 11 checks passed
@gabrielmer gabrielmer deleted the chore-deprecating-pubsub-topic branch September 10, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SionoiS SionoiS SionoiS approved these changes

@chaitanyaprem chaitanyaprem chaitanyaprem left review comments

@jm-clius jm-clius jm-clius approved these changes

@Ivansete-status Ivansete-status Ivansete-status approved these changes

@darshankabariya darshankabariya Awaiting requested review from darshankabariya

@NagyZoltanPeter NagyZoltanPeter Awaiting requested review from NagyZoltanPeter

@alrevuelta alrevuelta Awaiting requested review from alrevuelta

Assignees

@gabrielmer gabrielmer

Labels
E:nwaku Sharding peer management and discovery release-notes Issue/PR needs to be evaluated for inclusion in release notes highlights or upgrade instructions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

chore: remove --pubsub-topic configuration option
6 participants
@gabrielmer @NagyZoltanPeter @SionoiS @chaitanyaprem @jm-clius @Ivansete-status